-
Notifications
You must be signed in to change notification settings - Fork 655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Script::dust_value()'s calculation for non-P2*PKH script_pubkeys #579
Conversation
src/blockdata/script.rs
Outdated
3 * // The default dust relay fee is 3000 satoshi/kB (ie 1 sat/vByte) | ||
if self.is_op_return() { | ||
0 | ||
} else if self.is_witness_program() { | ||
Self::WITNESS_OUTPUT_DUST_THRESHOLD | ||
32 + 4 + 1 + (107 / 4) + 4 + // The spend cost copied from Core | ||
8 + // The serialized size of the TxOut's amount field | ||
self.consensus_encode(&mut ::std::io::sink()).unwrap() as u64 // The serialized size of this script_pubkey | ||
} else { | ||
Self::LEGACY_OUTPUT_DUST_THRESHOLD | ||
32 + 4 + 1 + 107 + 4 + // The spend cost copied from Core | ||
8 + // The serialized size of the TxOut's amount field | ||
self.consensus_encode(&mut ::std::io::sink()).unwrap() as u64 // The serialized size of this script_pubkey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will suggest improving readability of this fragment
if self.is_op_return() {
return 0
}
let script_pubkey_len = self.consensus_encode(&mut ::std::io::sink()).expect("I/O sink never fails") as u64;
let witness_reducer = (self.is_witness_program() as u8) * 4;
DEFAULT_DUST_RELAY_FEE * // The default dust relay fee is 3000 satoshi/kB (ie 1 sat/vByte)
(32 + 4 + 1 + 107 / witness_reducer + 4 // The spend cost copied from Core
+ 8 // The serialized size of the TxOut's amount field
+ script_pubkey_len)
And introduce DEFAULT_DUST_RELAY_FEE
const additionally to other values introduced by #584
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that DUST_RELAY_FEE
is a useful constant - it is only used when calculating the dust threshold, not in policy in any other context. I'm not sure what a user would do with such a constant. As for your proposed change, note that witness_reducer
can be 0 causing a divide-by-0 :).
The current code was written so as to make it clear its directly comparable to Core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 45d48c5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's quite important to get this bugfix in the upcoming 0.26.1, so I withdraw my nits and do a second ACK to this PR such that it can be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/blockdata/script.rs
Outdated
/// broadcastable on today's bitcoin network. | ||
pub const WITNESS_OUTPUT_DUST_THRESHOLD: u64 = 294; | ||
pub const P2WPKH_OUTPUT_DUST_THRESHOLD: u64 = 294; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these constants actually still all that useful now? If we fell for it someone else might do the same and just use one of the constants, not paying attention to the script type 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, admittedly, I'm happy to remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do if you want, I don't have too strong feelings either way. It just seemed somewhat redundant and I wanted to bring up the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do, in fact. Pushed.
The dust calculations added were only valid for P2WPKH and P2PKH outputs, and somehow this fact was missed in review, despite the upstream Core code being linked to and looked at by two reviewers and the author (me). Someday I will grow eyeballs, but that day is not today.
15981c9
It doesn't really make sense to have a constant for every common script type's dust limit, instead we should just use the `Script::dust_value()` function to have users calculate it.
Ok, sorry about the churn, this should be good now, assuming we agree to drop the useless constants. |
If we drop the constants this is a breaking change. |
Right, didn't think of that 😬 so maybe only deprecate now and remove with 0.27.0? |
I think released 0.26.0 hadn't those constants yet, so removing them shouldn't be breaking |
Indeed, none is this has appeared in a release yet, so I figured it was fine to change. |
Ah! My bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack fc6f23f
The dust calculations added were only valid for P2WPKH and P2PKH
outputs, and somehow this fact was missed in review, despite the
upstream Core code being linked to and looked at by two reviewers
and the author (me).
Someday I will grow eyeballs, but that day is not today.